Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update plugins.ts to support zackad/prettier-plugin-twig #327

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

bitbirddev
Copy link

@bitbirddev bitbirddev commented Nov 2, 2024

resolves #254

@alessandro-newzoo
Copy link

@bitbirddev thank you for this! 🙏

@thecrypticace please I haven't been able to update the plugin for a while due to this simple change not being implemented yet, could you please take a look and merge this PR if poissible?

References:
#308
#254 (comment)

Thank you

@darylknight
Copy link

@adamwathan Please could you review this?

@thecrypticace
Copy link
Contributor

The tests in this PR are currently failing — we've been focused on some other things so I haven't looked into why yet. That's the biggest reason this hasn't been merged.

@florianbouvot
Copy link

@bitbirddev @zackad can you fix the tests to allow @thecrypticace to merge this PR?

@zackad
Copy link

zackad commented Dec 29, 2024

I don't even know what the problem is. Been tinkering in the last few weeks. I can't even make the test pass from main branch in my setup. I use NixOS and macos with home-manager, same result.

When using docker, the test fail only when loading twig plugin. Sorry but I don't have patient to debug on docker setup.

In the meantime, I use my own custom build of this plugin with patch 521576a applied on top.

@thecrypticace
Copy link
Contributor

Yeah I spent a little bit time on it a while ago but couldn't figure out what the main cause was. I might see if I can pin it down this week. I was quite surprised that the upgrade caused a few of these tests to fail.

@thecrypticace
Copy link
Contributor

thecrypticace commented Jan 3, 2025

Okay I spent some time today and I figured it out! There was a behavior change when parsing certain expressions into an AST in the newer package.

Previously this code:

<section class="sm:p-0 p-0 text-{{ i }}"></section>

would get parsed into something structurally similar to this:

-> Element (`section`)
  -> Attribute (`class`)
    -> BinaryConcatExpression (`~`)
      -> StringLiteral (`sm:p-0 p-0 text-`)
      -> Identifier (`i`)

which meant we could reliably sort the string sm:p-0 p-0 text- and handle the fact that it is a concatenation

But now its parsed into someething like this:

-> Element (`section`)
  -> Attribute (`class`)
    -> StringLiteral (`sm:p-0 p-0 text-{{ i }}`)

So we see the string literal, go to sort it, and find that it has {{ in it. We bail in this case to match the behavior of Prettier itself (also saves us from having to do some manual parsing of the string literal).

That means that the test failing here is expected and should just be removed for zackad/prettier-plugin-twig assuming that the parsing behavior is correct. (cc @zackad I assume you might be able to provide some insight here)

I'll get stuff merged next week and tag a new release.

@zackad
Copy link

zackad commented Jan 4, 2025

I think the former behaviour is the correct one. From my investigation, this changes is introduced by zackad/prettier-plugin-twig#38. I'll try to revert this behaviour.

I've publish v0.14.1 that should fix this regression. Thanks for your insight.

@thecrypticace
Copy link
Contributor

Awesome thanks @zackad I've bumped the dev dependency and re-enabled the tests — appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add support for zackad/prettier-plugin-twig-melody
6 participants